Skip to content

Fix PositionGroup.fetch_position_info() returning empty DataFrame when merge IDs are non-chronological #1475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Fix PositionGroup.fetch_position_info() returning empty DataFrame when merge IDs are non-chronological #1475

wants to merge 4 commits into from

Conversation

Copy link
Contributor

Copilot AI commented Nov 20, 2025 •
edited
Loading

PositionGroup.fetch_position_info() returns an empty DataFrame when position merge IDs are fetched in non-chronological order (e.g., alphabetically by UUID rather than by time). Pandas .loc[min:max] slicing on an unsorted index returns empty results.

Changes

  • Core fix: Add .sort_index() before .loc[] slice in fetch_position_info() (line 232)
  • Test coverage: Add test_position_group_non_chronological_order() to verify sorted output

Example

Without the fix, concatenating non-chronological position data fails:

# Sleep session (UUID: 141c4c08-..., time: 10-20s) fetched first
# Run session (UUID: a7585350-..., time: 0-5s) fetched second
# Concatenated index: [10, 15, 20, 0, 2.5, 5]

df.loc[0:20]  # Returns empty - index not sorted
df.sort_index().loc[0:20]  # Returns all data

The fix ensures correct behavior regardless of fetch order:

position_info = (
    pd.concat(position_info, axis=0).sort_index().loc[min_time:max_time]
)
Original prompt

This section details on the original issue you should resolve

<issue_title>PositionGroup.fetch_position_info() breaks if merge ids order does not match chronological order of the positions</issue_title>
<issue_description>I'm running into an issue where PositionGroup.fetch_position_info() returns an empty dataframe and have narrowed it down to what I think is a bug at the end of the fetch_position() function.

The two pos_merge_ids that I'm trying to combine into a position group are as follows:
run session 04_r2 (happens first): UUID('a7585350-6b4b-8f46-b4c9-e350ecffc1b6')
sleep session 05_s3 (happens second): UUID('141c4c08-ec23-114f-ebbf-0168fc3b2b42')

Although I pass these keys into PositionGroup().create_group() via a list of dicts in the above order, they end up getting iterated through in alphabetically-sorted order in PositionGroup.fetch_position() which results in the position_info dataframe containing the sleep position info (chronologically later) first and the run position info (chronologically earlier) second. Because of this, once the dataframes are concatenated and indexed from [min_time: max_time] (see line 231 in spyglass.decoding.v1.core.py), this returns an empty dataframe.

Instead, I think the concatenated dataframe should be sorted by the time index prior to doing the min_time, max_time indexing.

I've replaced the original line 231 with
position_info = pd.concat(position_info, axis=0).sort_index().loc[min_time:max_time] in my own code and that solved the issue for me!</issue_description>

<agent_instructions>Use TDD. </agent_instructions>

Comments on the Issue (you are @copilot in this section)

@edeno That makes sense. Thank for filing the issue!

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@edeno edeno requested a review from Copilot November 20, 2025 20:21
Copilot AI changed the title [WIP] Fix fetch_position_info() to respect merge ids order Fix PositionGroup.fetch_position_info() returning empty DataFrame when merge IDs are non-chronological Nov 20, 2025
Copilot finished work on behalf of edeno November 20, 2025 20:22
Copilot AI requested a review from edeno November 20, 2025 20:22
Copilot finished reviewing on behalf of edeno November 20, 2025 20:23
Copilot AI reviewed Nov 20, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in PositionGroup.fetch_position_info() where concatenated position data from multiple merge IDs could return an empty DataFrame when the merge IDs were not fetched in chronological order.

Key Changes:

  • Added .sort_index() to ensure position data is chronologically sorted before time-based slicing
  • Created test to verify the fix prevents empty DataFrames and ensures monotonically increasing time indices

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/spyglass/decoding/v1/core.py Added .sort_index() call before .loc[min_time:max_time] slice to ensure concatenated position data is sorted chronologically
tests/decoding/test_core.py Added test test_position_group_non_chronological_order to verify position data is properly sorted and non-empty after fetching
Comments suppressed due to low confidence (2)

tests/decoding/test_core.py:2

  • Import of 'pd' is not used.
import pandas as pd

tests/decoding/test_core.py:3

  • Import of 'np' is not used.
import numpy as np

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Nov 20, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.12%. Comparing base (32b6950) to head (7336490).

Files with missing lines Patch % Lines
src/spyglass/decoding/v1/sorted_spikes.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1475   +/-   ##
=======================================
  Coverage   71.12%   71.12%           
=======================================
  Files         114      114           
  Lines       13306    13306           
=======================================
  Hits         9464     9464           
  Misses       3842     3842
Flag Coverage Δ
full-tests 71.12% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

edeno and others added 2 commits November 21, 2025 07:06
@edeno
Update tests/decoding/test_core.py
4dcb249 
Co-authored-by: Copilot <[email protected]>
@edeno
Ensure DataFrame index is sorted before slicing
7336490 
Added sort_index() before .loc[min:max] slicing in clusterless.py, core.py, and sorted_spikes.py to prevent empty DataFrame results when indices are unsorted. Includes a regression test in test_core.py to verify correct behavior and address issue #1471.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@edeno edeno Awaiting requested review from edeno

At least 1 approving review is required to merge this pull request.

Assignees

@edeno edeno

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PositionGroup.fetch_position_info() breaks if merge ids order does not match chronological order of the positions

2 participants

@edeno